-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow tasks to be declared directly under Locust classes #1304
Conversation
* Let users specify tasks directly under a Locust class, just as one would do it on a TaskSet class. These tasks will get the Locust instance as argument when executed. * Removed Locust.task_set from the public API, and instead let users use either the tasks attribute or the @task decorator. * Introduce a Locust.abstract boolean attribute. If it's set to True the Locust class is meant to be used as a base class, and users of that type can not be spawned directly from it.
Hm, I meant to create a draft PR until I had updated the docs, and Github doesn't seem to support changing it to draft. Oh well. |
Codecov Report
@@ Coverage Diff @@
## master #1304 +/- ##
==========================================
+ Coverage 79.73% 79.82% +0.09%
==========================================
Files 23 23
Lines 2092 2112 +20
Branches 325 327 +2
==========================================
+ Hits 1668 1686 +18
- Misses 338 341 +3
+ Partials 86 85 -1
Continue to review full report at Codecov.
|
Cool stuff! Looks good! Just one question, in what cases would an abstract locust class have tasks defined? |
I could see a case where you'd want to simulate two kinds of users that have much behaviour in common. Then that behaviour could be defined in tasks on an abstract base class that two different non-abstract Locust classes inherits from. Then there's the other way around - that you want a non-abstract Locust class without any tasks pre-declared on the class, and instead populate the |
Ignore my last (deleted) comment, I misunderstood you :) Ok, I understand the use case now. Feels a bit weird, but it is not a big thing. I'm +1! |
Though I do have one issue (and I think this was one I raised before): Wont users be confused by the fact that the self-parameter when their "on-locust" tasks are called is a TaskSet instance (made from a the somewhat secret DefaultTaskSet class) and not an instance of their Locust class? If they are only accessing things like self.client that are just wrapped by TaskSet then it is not so bad, but if they for some reason add an instance property to their Locust class (in |
Very valid concern I think. This code in the Lines 432 to 438 in af1e5e8
|
Ah, cool, I didnt see that (in fact github hid the whole diff for core.py because it was too big :) ) 👍 |
Dont merge this right now, I think I have found a bug... |
For some reason the code now instantiates the base class (e.g. HttpLocust), not the user defined one (MyHttpLocust). So any class variables defined by the user are unavailable. I couldnt really figure out what is the reason... We should add a runner test case for this I think. |
Oh, interesting. Do you have a way to reproduce it? I'm still working on updating the docs, and won't merge it until that's ready (and also not before the bug is fixed obviously). |
…der Locust classes. Some general documentation improvements here and there.
All documentation has now been updated to reflect the new API (I think, though I might have missed something). I also did some various improvements to the documentation that I found while going through all of the docs. I feel ready to merge :) |
Two last things...
Other than that it looks great! |
…d coding DefaultTaskSet as far as I can tell
… type TaskSet (that hasn't been decorated by @task)
True, I see no point in having the ability to change the DefaultTaskSet. Fixed now!
I'm ok with breaking changes for 1.0, but emitting a warning is not much extra job and might be a nice touch for people who don't read the migration guide meticulously, so I added it :). |
This PR changes how tasks and TaskSets are declared on Locust classes. It's related to #1264 and an alternative approach to #1274. It sprung from multiple discussions with @cyberw around ways to simplify Locust for very simple cases.
All tests pass, but the documentation would still need updating before this could be merged.
The PR contains the following changes:
Let users specify tasks directly under a Locust class, just as one would do it on a TaskSet class. These tasks will get the Locust instance as argument when executed.
Example:
or:
or as a dict with weights:
Removes
Locust.task_set
from the public API, and instead let users use either the tasks attribute or the @task decorator to declare tasks and/or TaskSets on Locust user classes (see example above).Introduce a
Locust.abstract
boolean attribute. If it's set to True the Locust class is meant to be used as a base class, and users of that type can not be spawned directly from it. This replaces the current way of considering a Locust class non-abstract if it has atask_set
attribute set. (This could have been a separate PR, but it's a fairly small change that is definitely an improvement, and it would have taken quite a bit of time for me to extract those small parts into a separate PR).